BUG: Rolling negative window issue fix #13383 #13441
Conversation
priyankjain
force-pushed
the
novice-bug-fixes
branch
from
0cd22d6 to
e000807
Compare
Current coverage is 84.32%@@ master #13441 diff @@
==========================================
Files 138 138
Lines 51068 51072 +4
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 43065 43069 +4
Misses 8003 8003
Partials 0 0
|
| @@ -850,6 +850,8 @@ def validate(self): | |||
| super(Rolling, self).validate() | |||
| if not com.is_integer(self.window): | |||
| raise ValueError("window must be an integer") | |||
| elif self.window < 0: | |||
There was a problem hiding this comment.
there is a another place where this is checked. in _Window (so need a test for that as well)
priyankjain
force-pushed
the
novice-bug-fixes
branch
from
e000807 to
cb92cba
Compare
| @@ -320,7 +320,7 @@ def validate(self): | |||
| window = self.window | |||
| if isinstance(window, (list, tuple, np.ndarray)): | |||
| pass | |||
| elif com.is_integer(window): | |||
| elif com.is_integer(window) and window >= 0: | |||
There was a problem hiding this comment.
do this inside the elif
elif com.is_integer(window):
if window < 0:
raise ......
...
cb92cba to
7c14e29
Compare
| try: | ||
| import scipy.signal as sig | ||
| except ImportError: | ||
| raise ImportError('Please install scipy to generate window' |
There was a problem hiding this comment.
With this change, the test case
series.rolling(0, win_type='boxcar') fails,
should we change travis configuration file (requirements-3.5_OSX.build) to have scipy installation as part of the build or change the test case to handle the import error? @jreback
There was a problem hiding this comment.
change the test case to skip if scipy is not installed
priyankjain
force-pushed
the
novice-bug-fixes
branch
from
7c14e29 to
2f79217
Compare
| @@ -1,2 +1,3 @@ | |||
| numpy=1.10.4 | |||
| cython | |||
| scipy | |||
There was a problem hiding this comment.
no, this is left off on purpose
priyankjain
force-pushed
the
novice-bug-fixes
branch
3 times, most recently
from
9da3800 to
967ffb8
Compare
| @@ -339,6 +344,13 @@ def test_constructor(self): | |||
| c(window=2, min_periods=w) | |||
| with self.assertRaises(ValueError): | |||
| c(window=2, min_periods=1, center=w) | |||
| # GH 13383 | |||
There was a problem hiding this comment.
make this a separate test (as skipping will cause the entire rest of the test to be skipped)
priyankjain
force-pushed
the
novice-bug-fixes
branch
from
967ffb8 to
26c9b2d
Compare
|
thanks! |
git diff upstream/master | flake8 --diffAdded functionality in validate function of Rolling to ensure that window size is non-negative.